Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor GHAs #124

Merged
merged 8 commits into from
Aug 27, 2024
Merged

Refactor GHAs #124

merged 8 commits into from
Aug 27, 2024

Conversation

iowillhoit
Copy link
Contributor

@iowillhoit iowillhoit commented Aug 1, 2024

Refactors a lot of our GHAs to pass inputs through the env.
A lot of other random cleanup. I have added a lot of comments on this PR to help with any confusion.

@W-16223935@

@@ -59,5 +59,7 @@ runs:
SF_CHANGE_CASE_TEMPLATE_ID: ${{ inputs.SF_CHANGE_CASE_TEMPLATE_ID}}
SF_CHANGE_CASE_CONFIGURATION_ITEM: ${{ inputs.SF_CHANGE_CASE_CONFIGURATION_ITEM}}

- run: echo "case id is ${{ steps.ctc.outputs.ctcId }}"
- run: echo "[INFO] Change Case ID is:\ $STEPS_CTC_CTCID"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: You need to escape the space after a : if you want it to be valid yaml. Or put it on a new line...

- run: |
  echo "[INFO] Change Case ID is: $STEPS_CTC_CTCID"

@@ -46,7 +46,9 @@ runs:
yarn tsc
yarn oclif readme \
--no-aliases \
--version ${{ steps.next-version.outputs.tag }} \
--version "$STEPS_NEXT_VERSION_TAG" \
${{ inputs.multi == 'true' && '--multi' || '' }} \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expressions that render values that don't include the input are fine.

}
} catch(err) {
core.setFailed(err)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had issues reading nested values using jq and env vars. Rewrote this in javascript.

  • Passing:
    • package.json version (run)
    • package.json devDependencies.@salesforce/dev-scripts (run)
  • Failing:
    • package.json versionzzz (run)
    • package.jsonssss version (run)

@@ -1,22 +0,0 @@
# This action is only needed as long as the developer site and other docs are linking to the old sfdx.pkg file and the mac signing job is only signing the old file as well.
# It can be deleted once those are updated to use the new name, sfdx-x64.pkg.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer being used

run: sf-release dependabot:automerge --merge-method ${{ inputs.mergeMethod}} --max-version-bump ${{ inputs.maxVersionBump}} --owner $GITHUB_REPOSITORY_OWNER --repo $(basename $GITHUB_REPOSITORY)
- name: run automerge command
if: ${{ inputs.skipCI }}
run: sf-release dependabot:automerge --merge-method "$INPUTS_MERGE_METHOD" --max-version-bump "$INPUTS_MAX_VERSION_BUMP" --owner $GITHUB_REPOSITORY_OWNER --repo $(basename $GITHUB_REPOSITORY) ${{ inputs.skipCI && '--skip-ci' || '' }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff is confusing. I turned two steps into one using a "ternaryish"

@@ -16,8 +16,6 @@ jobs:
runs-on: static-ip-ubuntu-runners
steps:
- uses: actions/checkout@v4
with:
token: ${{ secrets.SVC_CLI_BOT_GITHUB_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea why we would need our token for this workflow. The GH_TOKEN generated for the GHA run should work fine.

@@ -12,14 +12,14 @@ jobs:
runs-on: static-ip-ubuntu-runners
steps:
- uses: actions/checkout@v4
with:
token: ${{ secrets.SVC_CLI_BOT_GITHUB_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, GH_TOKEN should work

- uses: salesforcecli/github-workflows/.github/actions/get-json-property@main
id: packageVersion
with:
path: "package.json"
prop_path: 'devDependencies["@salesforce/dev-scripts"]'
prop_path: 'devDependencies.@salesforce/dev-scripts'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes is because I rewrote this action using javascript. I could not find any other uses of this action that used a nested value.

env:
STEPS_PACKAGE_VERSION_PROP: ${{ steps.packageVersion.outputs.prop }}

- run: echo "[INFO] shouldUpdate value will be:\ ${{ !endsWith(steps.packageVersion.outputs.prop, steps.version-info.outputs.version) }}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expression will render to either true or false

- run: npm install -g yarn-deduplicate
- run: yarn upgrade @salesforce/dev-scripts@latest
# TODO: What does this mean? Would yarnInstallWithRetries work here?
# this may fail because that's how dev-scripts does things
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two open questions here (TODO), would appreciate some feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Do we need this? Isn't that why we use npx?

Not sure why, yarn-deduplicate can be executed as a global binary. I can remove the call via npx.

TODO: What does this mean? Would yarnInstallWithRetries work here?

I'm pretty sure it's because of sf-install, when it updates dependencies it throws (exit code 1) and ask you to re-run yarn install, so the first yarn install continues on error to allow the second one (if the second one fails then the workflow exits):
https://github.com/forcedotcom/dev-scripts/blob/6bd069d98d14e8d1bfd629c720b8e5c87b8a45ed/bin/sf-install.js#L15

- name: NUTs with ${{ inputs.attempts }} attempts
uses: salesforcecli/github-workflows/.github/actions/retry@main
with:
max_attempts: ${{ inputs.attempts }}
command: ${{ inputs.command }}
retry_on: error
env:
TESTKIT_EXECUTABLE_PATH: ${{ inputs.sfdxExecutablePath }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted the step above that sets this. I think setting it here on the env will work exactly the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the previous step was supposed to set the env var only if inputs.sfdxExecutablePath was set, I'm not sure if this is equivalent since the env var could be set to an empty string or undefined and make testkit search for an invalid CLI bin, I'll test on QA to confirm.

- name: Is published
id: is-published
run: |
RESPONSE=$(npm view .@${{ inputs.githubTag }} version --json --silent || echo "Not published")
RESPONSE=$(npm view .@$INPUTS_GITHUB_TAG version --json --silent || echo "Not published")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this locally to make sure the env var is evaluated correctly 👍

- name: NUTs with ${{ inputs.retries }} attempts
uses: salesforcecli/github-workflows/.github/actions/retry@main
with:
max_attempts: ${{ inputs.retries }}
command: ${{ inputs.command }}
retry_on: error
env:
TESTKIT_EXECUTABLE_PATH: ${{ inputs.sfdxExecutablePath }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here (see comment above)

@@ -20,28 +16,39 @@ on:
jobs:
macos:
env:
AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the AWS secrets to only the steps that need them. Otherwise they will be set on the env for the entire job.

@@ -20,28 +16,39 @@ on:
jobs:
macos:
env:
AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
SFDX_HIDE_RELEASE_NOTES: true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- uses: salesforcecli/github-workflows/.github/actions/yarnInstallWithRetries@main
# todo: download the macos tarball and pass that to the oclif pack:macos command
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided this did not provide any real benefit and already never'd a ticket

cli:
type: string
required: true
description: only needed if upload. sfdx or sf
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more sfdx 🎉 These inputs were removed here

@@ -1,10 +1,6 @@
on:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file are the same as packUploadMac.yml. See those comments

@@ -7,36 +7,41 @@ on:

jobs:
publish:
# todo: parameterize this as input if anyone ever needs ip-restricted runner
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a priority

clipkg:
type: string
description: npm package name for cli
default: "@salesforce/cli"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more sfdx 🎉 These inputs were removed here

SF_DISABLE_TELEMETRY: true
runs-on: ubuntu-20-8core
steps:
- uses: actions/checkout@v4
with:
token: ${{ secrets.SVC_CLI_BOT_GITHUB_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not sure why we would need our token here

STAMPY_UNSIGNED_BUCKET: ${{ secrets.STAMPY_UNSIGNED_BUCKET }}
AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
AWS_EC2_METADATA_DISABLED: true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confusing diffs in this file

  • Added a name
  • Moved env to the bottom for consistency
  • Moved inputs and steps to the env

Copy link
Member

@cristiand391 cristiand391 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good overall, left some comments about things I'll test on QA.

env vars are correctly wrapped in double quotes.

notes for myself:
see if command substitution still works as input -> env var -> echo

@cristiand391
Copy link
Member

cristiand391 commented Aug 21, 2024

QA notes:

✅ getPreReleaseTag
✅ npmPublish
✅ create-github-release (tested with prereleases)
❌ NUTs fail:
https://github.com/salesforcecli/plugin-data/actions/runs/10492620689/job/29064652653

I think it's because the checkout happens after setup-node:
https://github.com/search?q=repo%3Aactions%2Fsetup-node%20%2FSupported%20file%20patterns%2F&type=code

✅ doesn't execute ls or $(ls) when printing from output via env var (also couldn't find any workflow that picks up user-content and tries to echo it (validatePr is already fixed)

printing output directly does run the command:
https://github.com/iowillhoit/gha-sandbox/actions/runs/10374680197/job/28722650085

@iowillhoit iowillhoit merged commit b171803 into main Aug 27, 2024
1 check passed
@iowillhoit iowillhoit deleted the ew/input-envs branch August 27, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants